-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
eslint-plugin: Add rule no-unused-vars-before-return
#12828
Conversation
ESLint version was bumped because it adds support for the npm organization scope for reference in rules. |
Should the ESLint version also be bumped in
|
@ntwb It's an included change: gutenberg/packages/scripts/package.json Line 41 in fddb736
|
It certainly works as expected on Travis :)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works great. I'm not super familiar with Eslint API and stuff, but the implementation looks good as far as I can tell.
We should add documentation which explains how this new rule work and changelog entry. How do you plan to tackle all errors? Should we open another PR and merge it sooner?
@@ -0,0 +1 @@ | |||
module.exports = require( 'requireindex' )( __dirname ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it equivalent to re-exporting all files in the folder? just being curious :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s equivalent to something like:
module.exports = {
foo: require( './foo' ),
bar: require( './bar' ),
baz: require( './baz' ),
};
Ah right. That would be nice to have an infrastructure in place for rule's verification in general. I assume we will have more rules in the future. This might be similar to what we use for Babel preset: https://github.com/WordPress/gutenberg/blob/master/packages/babel-preset-default/test/index.js |
fddb736
to
6777085
Compare
I've rebased this to bring it up to date, and have added documentation / tests for the new rule which can hopefully serve as precedent for future rule additions. |
6777085
to
cc4642f
Compare
I separately addressed the style issues for BlockSwitcher in #13431 . In the rebased branch, there are no instances of offending code in the codebase. Other changes included in the rebase:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I really like this rule as it helps to keep functionality close to the place where it is actually used. Great work on this custom rule 💯
@@ -0,0 +1,31 @@ | |||
# Avoid assigning variable values if unused before a return (no-unused-vars-before-return) | |||
|
|||
To avoid wastefully computing the result of a function call when assigning a variable value, the variable should be declared as late as possible. In practice, if a function includes an early return path, any variable declared prior to the `return` must be referenced at least once. Otherwise, in the condition which satisfies that return path, the declared variable is effectively considered dead code. This can have a performance impact when the logic performed in assigning the value is non-trivial. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reads nicely 👍
packages/scripts/CHANGELOG.md
Outdated
|
||
### Breaking Changes | ||
|
||
- The bundled `eslint` dependency has been updated from requiring `^4.19.1` to requiring `^5.12.1`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also bump Babel
at the same time and apply some tweaks to the preset we ship in the same version bump. I will open another PR with changes proposed.
|
||
context.report( | ||
node, | ||
`Declared variable \`${ variable.name }\` is unused before a return path` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite sure that is going to be clear enough when printed on the console. Should it include more detailed info that explains that there is a return path where it is not used before it is even used? I doesn't read good but I hope it explain my concern. Anyway, I don't find it as blocker :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I played with the message a bit more. One thing which confused me is its placement on the return
statement vs. the assignment itself, which I've found can be adjusted. Though there's also some value in having it on the actual return which flags it as offending 🤷♂️
Any thoughts on (attached to the variable declaration):
Variables should not be assigned until just prior its first reference. An early return statement may leave this variable unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is much better. Let's proceed with it 👍
I pushed the fix which should fix the failing test: 31a46bf. Unit tests needed to be updated to reflect the change in the rule error message. |
…rnmobile/372-use-RichText-on-Title-block * 'master' of https://github.com/WordPress/gutenberg: Try alternate list item jump fix. (#12941) Mobile bottom sheet component (#13612) Remove unintentional right-margin on last odd-item. (#12199) Introduce left and right float alignment options to latest posts block (#8814) Fix Google Docs table paste (#13543) Increase bottom padding on gallery image caption (#13623) Fix the editor save keyboard shortcut not working in code editor view (#13159) Plugin: Deprecate gutenberg_add_admin_body_class (#13572) Rnmobile/upload media failed state (#13615) Make clickOnMoreMenuItem not dependent on aria labels (#13166) Add: className prop support to server side render. (#13568) Fix: Categories Block: hierarchical Dropdown (#13567) Docs: Add clarification about git workflow (#13534) Plugin: Remove `user_can_richedit` filtering (#13608) eslint-plugin: Add rule `no-unused-vars-before-return` (#12828) Image settings button (#13597) Fixed wording for the color picker saturation (#13479) # Conflicts: # packages/block-library/src/image/edit.native.js
* scripts: Bump ESLint dependency to 5.10.x * eslint-plugin: Add rule `no-unused-vars-before-return` * eslint-plugin: Rephrase return-unused message for declarator * Update error message in the unit test to reflect code changes * Scripts: Include Migration Guide link for ESLint major bump CHANGELOG note
* scripts: Bump ESLint dependency to 5.10.x * eslint-plugin: Add rule `no-unused-vars-before-return` * eslint-plugin: Rephrase return-unused message for declarator * Update error message in the unit test to reflect code changes * Scripts: Include Migration Guide link for ESLint major bump CHANGELOG note
Related: #12477
Blocked by #12827(since merged)This pull request seeks to introduce a new ESLint rule
@wordpress/no-unused-vars-before-return
. This rule captures potential performance issues related to the assignment of a variable from the result of a function call expression, where the declared variable is not referenced prior to a possible return path of the function.Practically speaking, this looks like:
Refer also to modified code in #12827 as example problematic code.
Try yourself: https://astexplorer.net/#/gist/43c7ba92c2ea5bce1cfdd870c0ad6c4e/432215c7751e8dcd2d50463c23459abd661c7572
Testing instructions:
(This is blocked by #12827 - You will observe lint errors in the meantime)Ensure there are no lint errors:
Pending tasks: